WIP: Optional feature: More verbose failed expression reporting #114
Conversation
I have solved this problem by introducing the so-called markers, which help me to exclude the failed nodes that should not be printed. I hope that my code, in spite of it being messy, is easy to understand. What I also found is that it could even make sense to print everything of Match* that is found in the deque because it gives a very nice presentation of what the grammar is capable of within the context of a currently edited file. |
stanislaw
force-pushed
the
stanislaw/circular4
branch
4 times, most recently
from
cf58a6f to
c05f908
Compare
There was a problem hiding this comment.
Thanks @stanislaw.
I did a first iteration of review where I focused more on the behavior than the implementation. I have several questions. Please see the comments.
I'll run some tests with real-world inputs to see if this new approach yields better/more intuitive reports. So far it seems to show a good potential.
| "Expected:\n" | ||
| "1:5: EOF\n" | ||
| "1:7: 'b'\n" | ||
| " => 'a, c*, '." |
There was a problem hiding this comment.
This one is nice as it shows the possibility of ending earlier.
| "'six' at position (1, 1) or " | ||
| "'4' at position (1, 15) or " | ||
| "'five' at position (1, 20) => " | ||
| "'*one two th'." |
There was a problem hiding this comment.
While this is technically correct I'm not sure how intuitive it will be for the user. I'll have to test with larger inputs to see what are the benefits. Maybe I'm wrong but I imagine if the input is large there will be many week failures and the report might get obscured as it will start reporting very early week failures. In addition, maybe the * marker should be at the last failure position. Just wondering.
There was a problem hiding this comment.
In addition, maybe the * marker should be at the last failure position. Just wondering.
I have changed this. Indeed, the * should be at the actual last failure's position.
| @@ -1413,7 +1563,7 @@ class Parser(DebugPrinter): | |||
| FIRST_NOT = Not() | |||
|
|
|||
| def __init__(self, skipws=True, ws=None, reduce_tree=False, autokwd=False, | |||
| ignore_case=False, memoization=False, **kwargs): | |||
| ignore_case=False, memoization=False, verbose=False, verbose2=False, **kwargs): | |||
There was a problem hiding this comment.
Why two different verbose flags? What is the purpose of each? Could we just use verbosity as a level (int).
|
@stanislaw Just a head up. I've been experimenting with this branch today. The experiment consisted of running examples ( The reports I got were quite unintuitive, at least for me. The master branch still provides a simple but more to the point report. What are your thoughts? Does this give any better messages in the context of StrictDoc? I still find much easier to understand what is going on by running the parser with |
stanislaw
force-pushed
the
stanislaw/circular4
branch
2 times, most recently
from
66f4b3a to
01166f5
Compare
An option is added to the Parser class. This option is disabled by default, and the existing behavior is fully preserved. When the option is enabled, the final expected message is extended with extra information about the previously "weakly failed" rules. This way, not only the last failed NoMatch exception and its failing rules are displayed but also all the rules that were not matched during the whole parsing process.
stanislaw
force-pushed
the
stanislaw/circular4
branch
from
01166f5 to
57a9003
Compare
The
verbose=False/Trueoption is added to the Parser class. This option is disabled by default, and the existing behavior is fully preserved.When the option is enabled, the final expected message is extended with extra information about the previously "weakly failed" rules. This way, not only the last failed NoMatch exception and its failing rules are displayed but also all the rules that were not matched during the whole parsing process.
Open points:
The markers could be removed, and all nodes could be printed whenImplemented byverbose=True.verbose2.verbosemay be not the best name for this option. I am happy to change it to any other name.Will be done after/if the approach is confirmed.
Code review checklist
CHANGELOG.md, no needto update for typo fixes and such).